Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. #15153

ferhatb · 2020-01-06T21:03:51Z

Added endOfPaint call on recording canvas to allow bitmap canvas to dispose after reuse.
Moved all immediate canvas calls from BitmapCanvas into CanvasPool.
Reduced context updates by creating ContextHandle class.
Added regression tests for drawing on top of image in a single Picture.

Fixes flutter/flutter#44845
Fixes flutter/flutter#48032
Fixes flutter/flutter#44585

yjbanov · 2020-01-06T21:06:27Z

lib/web_ui/lib/src/engine/bitmap_canvas.dart

-  String _prevFilter = 'none';
-  Object _prevFillStyle;
-  Object _prevStrokeStyle;
+  int _canvasPositionX, _canvasPositionY;


Like other fields, docs here would be useful.

yjbanov · 2020-01-06T21:33:29Z

lib/web_ui/test/engine/recording_canvas_test.dart

+      final RRect outer =
+      RRect.fromRectAndCorners(const Rect.fromLTRB(10, 20, 30, 40));
+      final RRect inner =
+      RRect.fromRectAndCorners(const Rect.fromLTRB(12, 22, 28, 38));


nit: the above two indents are off

yjbanov · 2020-01-06T21:38:30Z

lib/web_ui/lib/src/engine/canvas_pool.dart

+  // Fill a virtually infinite rect with the color.
+  void fill() {
+    html.CanvasRenderingContext2D ctx = context;
+    ctx.beginPath();


Why do we need this here but not in drawColor?

yjbanov · 2020-01-06T21:41:47Z

lib/web_ui/lib/src/engine/canvas_pool.dart

+    html.CanvasRenderingContext2D ctx = context;
+    contextHandle.blendMode = blendMode;
+    contextHandle.fillStyle = color.toCssString();
+    contextHandle.strokeStyle = '';


Still missing how a filter applied in a drawPaint won't leak into drawColor 🤔

Example:

c.drawPaint(... pass Paint with filter...); // at this point, because we didn't reset the previous paint the `filter` is still there. c.drawColor(... some color and blend mode ...);

Done. Added filter = 'none';

yjbanov · 2020-01-06T21:42:49Z

lib/web_ui/lib/src/engine/canvas_pool.dart

+  ui.StrokeCap _currentStrokeCap = ui.StrokeCap.butt;
+  ui.StrokeJoin _currentStrokeJoin = ui.StrokeJoin.miter;
+  Object _currentFillStyle;
+  Object _currentStrokeStyle;


Can you please leave a comment explaining why the above two fields need to be Object?

yjbanov · 2020-01-06T21:44:26Z

lib/web_ui/lib/src/engine/canvas_pool.dart

+    context.fillStyle = '';
+    _currentFillStyle = context.fillStyle;
+    context.strokeStyle = '';
+    _currentStrokeStyle = context.strokeStyle;


Can you please leave a comment explaining why you have to write an empty string then read it back from context rather than assign an empty string to everything?

yjbanov · 2020-01-06T21:46:08Z

lib/web_ui/test/engine/recording_canvas_test.dart

    });

    test('Inner RRect > Outer RRect', () {
      underTest.drawDRRect(rrect, rrect.inflate(1), somePaint);
      underTest.apply(mockCanvas);
      // Expect nothing to be called
-      expect(mockCanvas.methodCallLog.length, equals(0));
+      expect(mockCanvas.methodCallLog.length, equals(1));
+      expect(mockCanvas.methodCallLog.toString(), contains('endOfPaint'));


nit: a shorter version

expect(mockCanvas.methodCallLog.single.methodName, 'endOfPaint');

yjbanov · 2020-01-06T21:48:12Z

lib/web_ui/test/golden_tests/engine/canvas_draw_image_golden_test.dart

+
+  // Regression test for https://github.com/flutter/flutter/issues/44845
+  // Circle should draw on top of image not below.
+  test('Paints on top of image', () async {


Did you forget to include a test that tests [circle, image] sequence? I saw the test included in the goldens, but it's not in this PR 🤔

ferhatb · 2020-01-07T23:36:23Z

Merging on infra failure due to XCode install

…osition bug. (flutter/engine#15153)

flutter/engine@3f52888...a50f1ef git log 3f52888..a50f1ef --first-parent --oneline 2020-01-08 gw280@google.com Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} (flutter/engine#15118) 2020-01-07 ferhat@gmail.com Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. (flutter/engine#15153) 2020-01-07 ferhat@gmail.com Recover when browser throws on ImageElement.decode due to too many images (flutter/engine#15160) 2020-01-07 garyq@google.com Fix RectHeightStyle::kMax ascent computation bug (flutter/engine#15106) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC garyq@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

…bug. (flutter#15153) * Refactor BitmapCanvas. Fix image compositing bug. Allocate canvas lazily * Fix recording canvas test by restoring context save * Update recording canvas test for drawColor to show multiply blend

ferhatb added 2 commits January 6, 2020 12:55

Refactor BitmapCanvas. Fix image compositing bug. Allocate canvas lazily

8b30bfd

dartfmt

c6c58fa

ferhatb requested a review from yjbanov January 6, 2020 21:03

auto-assign bot requested a review from gw280 January 6, 2020 21:03

googlebot added the cla: yes label Jan 6, 2020

yjbanov approved these changes Jan 6, 2020

View reviewed changes

Address review comments

703d703

yjbanov approved these changes Jan 6, 2020

View reviewed changes

ferhatb added 6 commits January 6, 2020 15:02

update licenses_flutter for new canvas_pool.dart

98fbb63

Merge remote-tracking branch 'upstream/master' into draw_on_image6

9ab3c29

Update golden lock

12f0c20

Fix recording canvas test by restoring context save

540ad19

Update recording canvas test for drawColor to show multiply blend

e11799e

Fix missing rc.restore calls in test

5a617bf

ferhatb requested a review from yjbanov January 7, 2020 23:29

yjbanov approved these changes Jan 7, 2020

View reviewed changes

ferhatb merged commit 27a221d into flutter:master Jan 7, 2020

This was referenced Jan 7, 2020

Flutter Web does not drawing gradients/colors over a picture flutter/flutter#48216

Closed

dart:ui canvas.drawColor(color, BlendMode.clear) memory leak flutter/flutter#47513

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

3e4d668

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48372

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

6ccb8b9

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48374

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

3d82f5c

…osition bug. (flutter/engine#15153)

This was referenced Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48375

Closed

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48376

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

5a98e5d

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

101d5f3

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48377

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

10255a1

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48378

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

1e3da46

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48380

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

27a221d Refactor BitmapCanvas, lazily allocate canvas, fix image comp…

f2fc07d

…osition bug. (flutter/engine#15153)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine 3f52888b3bcf..a50f1ef56a05 (4 commits) flutter/flutter#48382

Merged

ferhatb deleted the draw_on_image6 branch January 9, 2020 22:57

perclasson mentioned this pull request Jan 24, 2020

FlareActor is not visible on web with master channel flutter/flutter#49429

Closed

mdebbar mentioned this pull request Sep 17, 2020

[web] Fix leaking canvas in text placeholder tests #21253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. #15153

Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. #15153

ferhatb commented Jan 6, 2020 •

edited

Loading

yjbanov Jan 6, 2020

ferhatb Jan 6, 2020

yjbanov Jan 6, 2020

ferhatb Jan 6, 2020

yjbanov Jan 6, 2020

ferhatb Jan 6, 2020

yjbanov Jan 6, 2020

ferhatb Jan 6, 2020

yjbanov Jan 6, 2020

yjbanov Jan 6, 2020

ferhatb Jan 6, 2020

yjbanov Jan 6, 2020

ferhatb Jan 6, 2020

yjbanov Jan 6, 2020

ferhatb commented Jan 7, 2020

Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. #15153

Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. #15153

Conversation

ferhatb commented Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferhatb commented Jan 7, 2020

ferhatb commented Jan 6, 2020 •

edited

Loading